fix: make StepRegistry atomic and raise on corruption by Quratulain-bilal · Pull Request #3922 · github/spec-kit

Quratulain-bilal · 2026-07-31T15:50:23Z

Problem

StepRegistry._load() silently returned empty defaults on corruption, causing installed steps to appear to vanish. save() wrote directly to the target file risking data loss.

Fix

_load() now raises OSError on symlinked paths and corrupted files. save() now uses atomic writes via mkstemp + os.replace.

…nt data loss 1. _load() now raises OSError on symlinked paths and corrupted files instead of silently returning empty defaults (matches WorkflowRegistry) 2. save() now writes to a temp file and atomically replaces, preventing data loss from truncated writes on crash

Copilot

Pull request overview

Makes StepRegistry fail closed on corruption and persist data atomically.

Changes:

Raises OSError for unsafe or corrupted registries.
Writes through a temporary file using os.replace.
Cleans up temporary files after failures.

Show a summary per file

File	Description
`src/specify_cli/workflows/catalog.py`	Hardens registry loading and saving.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Files reviewed: 1/1 changed files
Comments generated: 3
Review effort level: Balanced

mnriem

Please address Copilot feedback and fix test and lint errors

Verifies: - StepRegistry raises OSError on symlinked registry files - StepRegistry raises OSError on corrupted JSON - StepRegistry.save() uses atomic write via mkstemp + os.replace Addresses Copilot review feedback on github#3922.

…mic-save Resolve conflicts by taking main's split of workflows/catalog.py: the StepRegistry code in this branch belongs in workflows/step/catalog/_domain.py, so main's workflows/catalog/_domain.py is taken as-is instead of carrying a duplicate StepRegistry, and tests/test_workflows.py takes main's version with the branch's new tests re-added on top. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)

Port the branch's StepRegistry hardening to where main now keeps the class (workflows/step/catalog/_domain.py): _load raises OSError for a symlinked path, an unreadable file, unparseable JSON, or a non-registry shape instead of silently returning an empty registry, and save writes through a temp file plus os.replace so a crash cannot leave a half-written registry. The new constructor errors would otherwise surface as tracebacks, and the step commands used to reach their directory-symlink check only because the registry swallowed the fault. A shared _load_step_registry_or_exit helper now prints a clean error, and add/remove resolve the steps base directory first so the symlinked-directory message keeps its specific wording. Five tests that pinned the silent-reset behaviour are updated to the fail-loud contract, and the branch's three regression tests (symlink, corrupted JSON, atomic save) come along. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)

…mic-save Take main's refactor in the conflicted step files: the registry hardening in this branch lives in workflows/step/catalog/_domain.py, while main has moved the CLI-coupled logic into workflows/step/installer.py (resolve_steps_base_dir, StepInstallError, the install transaction) and rewritten workflow_step_remove around _remove_step_locked. tests/test_workflows.py merges with the branch's registry tests kept on top of main's version. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)

StepRegistry._load returned an empty registry for a symlinked path, an unreadable file, unparseable JSON, or a non-registry shape, so corruption reached callers as a silently reset registry. It now raises OSError with the registry path in the message; save() already writes atomically on main. Because the constructor now raises, translate the error where it surfaces: installer._load_step_registry maps OSError onto StepInstallError so the existing command handlers keep printing a clean error, and the new _helpers._load_step_registry_or_exit does the same for the step commands. workflow_step_remove resolves the steps base directory before loading the registry so the symlinked-directory message keeps its specific wording. Tests: the three fail-loud regression tests (symlinked file, corrupted JSON, atomic save) are kept, and five tests that pinned the silent-reset behaviour now assert the raised OSError instead. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)

Copilot

Copilot review overview

🟡 Changes recommended

Atomicity and new error-translation paths lack required regression coverage, and one source comment contains mojibake.

Review effort: Balanced
Findings: 3 · 1

Open (4)

Add CLI tests for corrupted registry error handling · New
Test OSError translation before installer mutation · New
Add atomicity regression test with injected write failure · New
Restore intended em dash in source comment · New

Resolved since last review (3)

The new constructor error is not caught by the step CLI entry points (_commands.py:3042, 3188,…
mkstemp creates a new inode with mode 0600; replacing an existing registry therefore silently…
The existing corruption tests still assert the old fallback behavior:…

mnriem · 2026-10-06T19:13:42Z

Please address Copilot feedback

Addresses 4 Copilot findings: 1. test_cli_list_exits_cleanly_on_corrupted_registry: CLI produces exit code 1 with clean error, no traceback 2. test_installer_translates_oserror_before_mutation: OSError is translated to StepInstallError before any mutation 3. test_save_failure_preserves_previous_bytes: injected write failure leaves previous bytes intact (atomicity regression) 4. Fixed mojibake em dash in command_list.py comment Assisted-by: GitHub Copilot (autonomous)

Copilot

🟡 Changes recommended

The installer regression test bypasses the changed installation paths and cannot verify that corruption is rejected before filesystem mutation.

1 open finding

Test bypasses installer and misses corruption rejection before filesystem mutat… · New

4 resolved since last review

🧠 Review effort: Balanced

mnriem · 2026-10-13T12:34:49Z

Please address Copilot feedback

Replaces the direct _load_step_registry() call with a full install_step_package() invocation using a valid package and a corrupted registry, asserting no destination or staging directory is created. Assisted-by: GitHub Copilot (autonomous)

Copilot

🟡 Changes recommended

Two regression assertions are overly broad or do not match the staging-directory naming convention.

1 open finding

Match the installer's actual staging directory prefix · New

1 resolved since last review

Test bypasses installer and misses corruption rejection before filesystem mutat…

Previously missed (1)

In code that hasn't changed since last review

Assert save() raises the specific StepValidationError type

tests/test_workflows.py:11489

This assertion accepts every Exception, so it cannot verify save()'s documented error contract and would still pass if an unrelated exception escaped. Since the write path wraps I/O failures as StepValidationError, assert that specific type.

🧠 Review effort: Balanced

mnriem · 2026-10-13T14:54:17Z

Please address Copilot feedback

The glob tmp* does not match _WORK_DIR_PREFIX (.speckit-step-install-), so a leaked staging directory would go unnoticed. Assisted-by: GitHub Copilot (autonomous)

Copilot

🔵 Needs a closer look

Bundle reference validation still suppresses registry corruption and can treat installed steps as absent.

0 open findings

1 resolved since last review

Match the installer's actual staging directory prefix

Previously missed (3)

In code that hasn't changed since last review

Propagate registry corruption instead of treating it as not installed

src/specify_cli/workflows/step/catalog/_domain.py:117

This still does not make corruption fail loudly for every reader: bundles/references.py:18-60 catches this OSError as a best-effort lookup failure and returns False. Consequently, bundle validate can still treat an installed step as absent (or merely emit an offline/catalog warning), reproducing the “steps appear to vanish” behavior described by this PR. Propagate or explicitly translate registry corruption in that caller instead of collapsing it into “not installed,” with a bundle-reference regression test.

Add remove regression test for corrupted registry

src/specify_cli/workflows/step/command_remove.py:26

Add a negative regression test for the remove command with a corrupted registry, asserting a clean exit and that the existing step directory remains untouched. This newly routed path precedes destructive removal, but only step list and installation are covered; the repository requires relevant prevented-behavior tests, and tests/specify_cli/workflows/step/test_command_remove.py is the mirrored public-command suite.

Assert save raises StepValidationError

tests/test_workflows.py:11489

Exception subsumes OSError, so this assertion accepts any ordinary exception and does not verify save()'s public failure contract. Since save() wraps write failures as StepValidationError, assert that exact type so a regression that leaks an unrelated exception cannot pass.

🧠 Review effort: Balanced

mnriem · 2026-10-13T16:32:46Z

Please address Copilot feedback

…mic-save

Copilot

🟡 Changes recommended

Malformed step entries can still bypass validation and trigger tracebacks, and two changed command paths lack regression coverage.

3 open findings

Validate registry step values are objects · New
Add corrupted-registry test for step info · New
Add corrupted-registry removal regression test · New

🧠 Review effort: Balanced

+            if not isinstance(data.get("steps"), dict):
+                raise OSError(
+                    f"Step registry at {self.registry_path} is corrupted: "
+                    "'steps' must be an object"
+                )


    safe_step_id = cli._escape_markup(str(step_id))

-    registry = StepRegistry(project_root)
+    registry = step_helpers._load_step_registry_or_exit(project_root)


+    steps_base_dir = step_helpers._resolve_steps_base_dir_or_exit(project_root)
+    registry = step_helpers._load_step_registry_or_exit(project_root)


mnriem · 2026-10-14T13:29:07Z

Please address Copilot feedback

Quratulain-bilal requested a review from mnriem as a code owner July 31, 2026 15:50

mnriem requested a balanced review from Copilot August 13, 2026 15:49

Copilot started reviewing on behalf of mnriem August 13, 2026 15:50 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/catalog.py Outdated

Comment thread src/specify_cli/workflows/catalog.py Outdated

Comment thread src/specify_cli/workflows/catalog.py Outdated

mnriem requested changes Aug 31, 2026

View reviewed changes

Quratulain-bilal added 5 commits September 2, 2026 00:33

test: add regression tests for StepRegistry atomic save

6414687

Verifies: - StepRegistry raises OSError on symlinked registry files - StepRegistry raises OSError on corrupted JSON - StepRegistry.save() uses atomic write via mkstemp + os.replace Addresses Copilot review feedback on github#3922.

mnriem requested a balanced review from Copilot October 6, 2026 18:22

Copilot started reviewing on behalf of mnriem October 6, 2026 18:23 View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/step/_helpers.py

Comment thread src/specify_cli/workflows/step/installer.py

Comment thread tests/test_workflows.py

Comment thread src/specify_cli/workflows/step/command_list.py Outdated

mnriem requested a balanced review from Copilot October 13, 2026 11:54

Copilot started reviewing on behalf of mnriem October 13, 2026 11:55 View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread tests/test_workflows.py Outdated

mnriem requested a balanced review from Copilot October 13, 2026 14:41

Copilot started reviewing on behalf of mnriem October 13, 2026 14:42 View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread tests/test_workflows.py Outdated

fix(tests): match installer staging prefix in corruption test

df3d9f9

The glob tmp* does not match _WORK_DIR_PREFIX (.speckit-step-install-), so a leaked staging directory would go unnoticed. Assisted-by: GitHub Copilot (autonomous)

mnriem requested a balanced review from Copilot October 13, 2026 16:20

Copilot started reviewing on behalf of mnriem October 13, 2026 16:21 View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/step-registry-ato…

a31c56c

…mic-save

mnriem requested a balanced review from Copilot October 14, 2026 12:37

Copilot started reviewing on behalf of mnriem October 14, 2026 12:37 View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

		steps_base_dir = step_helpers._resolve_steps_base_dir_or_exit(project_root)
		registry = step_helpers._load_step_registry_or_exit(project_root)

Conversation

Quratulain-bilal commented Jul 31, 2026

Problem

Fix

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnriem commented Oct 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

mnriem commented Oct 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

mnriem commented Oct 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

mnriem commented Oct 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

mnriem commented Oct 14, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants